Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix transition:name can be unicode #9822

Merged
merged 28 commits into from
Jan 26, 2024

Conversation

liruifengv
Copy link
Member

@liruifengv liruifengv commented Jan 25, 2024

Changes

Testing

Added e2e test.

Docs

Not need update.

Copy link

changeset-bot bot commented Jan 25, 2024

🦋 Changeset detected

Latest commit: d302ee9

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jan 25, 2024
@liruifengv liruifengv marked this pull request as draft January 25, 2024 07:43
@liruifengv liruifengv changed the title fix: fix transition:name can be unicode WIP: fix: fix transition:name can be unicode Jan 25, 2024
@liruifengv liruifengv changed the title WIP: fix: fix transition:name can be unicode fix: fix transition:name can be unicode Jan 25, 2024
@liruifengv liruifengv marked this pull request as ready for review January 25, 2024 08:11
packages/astro/src/runtime/server/transition.ts Outdated Show resolved Hide resolved
packages/astro/src/runtime/server/transition.ts Outdated Show resolved Hide resolved
packages/astro/src/runtime/server/transition.ts Outdated Show resolved Hide resolved
packages/astro/src/runtime/server/transition.ts Outdated Show resolved Hide resolved
packages/astro/src/runtime/server/transition.ts Outdated Show resolved Hide resolved
@martrapp
Copy link
Member

Great PR! Don't let my many comments put you off, I really appreciate your contribution!

@yisibl
Copy link

yisibl commented Jan 25, 2024

Also, need to consider emoji, additionally is there a test case to test them?

@martrapp
Copy link
Member

Also, need to consider emoji, additionally is there a test case to test them?

An idea for a e2e test case would be to read back the view-transition-name style property, e.g. test el.style.viewTransitionName. If the view-transition-name in the generated style sheet contains illegal chars, the viewTransitionName of the elements styles will be completely empty. In other cases it will yield the string as shown on the right side of the screenshots above, i.e. human readable. Escaped ascii characters would would be shown with there escaping slash but hex encoded code points will be shown as the character they represent.

liruifengv and others added 8 commits January 25, 2024 23:01
Co-authored-by: Martin Trapp <[email protected]>
Co-authored-by: Martin Trapp <[email protected]>
Co-authored-by: Martin Trapp <[email protected]>
Co-authored-by: Martin Trapp <[email protected]>
Co-authored-by: Martin Trapp <[email protected]>
Co-authored-by: Martin Trapp <[email protected]>
Co-authored-by: Martin Trapp <[email protected]>
@natemoo-re
Copy link
Member

natemoo-re commented Jan 25, 2024

I did find a potential package to handle this! cssesc was written by Mathias Bynens, who is very involved in standards work. He also wrote an article and built a hosted tool to explore this topic.

Although the package is quite old, I'm confident that the code is stable!

I'd suggest that we try to use a package for this, and fallback to implementing our own logic only if there isn't a package out there that meets our needs.

@martrapp
Copy link
Member

I did find a potential package to handle this!

Excellent catch! As you can see from the linked issue, we were looking for a lib like this but failed.

cssesc(name, {'isIdentifier': true});

seems to be exactly what we need here.

@liruifengv
Copy link
Member Author

I did find a potential package to handle this! cssesc was written by Mathias Bynens, who is very involved in standards work. He also wrote an article and built a hosted tool to explore this topic.

Although the package is quite old, I'm confident that the code is stable!

I'd suggest that we try to use a package for this, and fallback to implementing our own logic only if there isn't a package out there that meets our needs.

Thank you! I used the package. It works!

@martrapp
Copy link
Member

Wow over 20 conversation and at the end a one line change (+ tests and dependencies of course)
That was a fun trip, I enjoyed it!

Copy link
Member

@martrapp martrapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good to me now!

@martrapp martrapp merged commit bd880e8 into withastro:main Jan 26, 2024
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

transition:name can't use Chinese、Japanese and Korean, etc.
4 participants